-
Notifications
You must be signed in to change notification settings - Fork 551
PYTHON-1297 Convert to pytest for running unit tests #1215
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ytest failed with an inability to detect the start call to the Twisted reactor. Underlying cause was an assumption about ordering; this test fails if other tests have already run if those tests start the reactor due to a short-circuit in cassandra.io.twistedreactor that kicks in if the reactor has already been started. This isn't an issue when TestTwistedConnection goes first (which apparently was true for pynose) but fails otherwise.
To be clear about the goals: we should consider this done when we've seen good runs of the following:
|
There are nosetest references in .gitignore and setup.py also. |
Good call @bschoening .... I'll yank those too. |
Test build at this point is only showing a few new errors, most of which appear to be PYTHON-1389. I'm going to fix those quickly and take a stab at the setup.py changes @bschoening mentioned above. After that we should be good for a retest. |
A quick update on this. We're down to a reasonable number of test failures (38 as of this writing, which is way better than the several thousand we were seeing). We were seeing some failures due to PYTHON-1389 but I put a fix in for that which seems to have addressed the issue. Nearly all of the failures I'm seeing at the moment are covered by one of two tickets:
Both of these issues need to be addressed, but as of this writing I don't have any reason to believe they're related to the pytest conversion... so I'm inclined to not hold on merging this functionality due only to these issues. Opinions welcome. @bschoening do you think I'm off-base here? |
@absurdfarce so, pytests are pretty clean except for when using Python 3.12? If so, yes, I wouldn't hold of on merging. |
Yup, you've got the gist of it @bschoening. There are a few other failures from flaky tests and the asyncore tests need to have a version bound for Python 3.12 but outside of that and the issues mentioned above the tests look pretty good. I'm inclined to agree with you. It seems to make more sense to get this in sooner rather than later; that way we can use pytest to help address the other issues mentioned above. |
pass | ||
loop = twistedreactor.TwistedConnection._loop | ||
if not loop._reactor_stopped(): | ||
loop._cleanup() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test started failing after moving to pytest, apparently because the order of operations changed. It looks like the old infrastructure included implicit assumptions about when the reactor was started... and now that pytest executed tests in a different order those assumptions led to unexpected test failures. The goal of this change is to make the test more robust by forcing each test case to do a better job cleaning up after itself.
@@ -102,6 +102,9 @@ def maybe_start(self): | |||
self._thread.start() | |||
atexit.register(partial(_cleanup, weakref.ref(self))) | |||
|
|||
def _reactor_stopped(self): | |||
return reactor._stopped | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used in order-of-operations test fix discussed below
@@ -54,7 +62,8 @@ def tearDownClass(cls): | |||
except: | |||
pass | |||
|
|||
|
|||
has_asyncore = Version(platform.python_version()) < Version("3.12.0") | |||
@unittest.skipUnless(has_asyncore, "asyncore has been removed in Python 3.12") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not part of the pytest conversion... just a drive-by cleanup while we're here
print("Running eventlet tests") | ||
from eventlet import monkey_patch | ||
monkey_patch() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This monkey patch in the gevent/eventlet case is now handled in test infrastructure so we shouldn't need to support it as a CLI arg as well.
There's also the fact that I can't find any actual usage of this arg anywhere...
…ability to detect extension. Both asyncore and libev were modified to raise DependencyException when native libs couldn't be found but only one test_asyncore was updated to account for the new type.
Added Travis coverage for Python 3.11 in this ticket. Python 3.12 is (naturally) a bit trickier so we'll address that in PYTHON-1392. With green builds everywhere except Jenkins (and those failures all corresponding to existing tickets or known issues) I'm calling this good. |
This branch represents a unification of separate work that has been done by @bschoening in #1171 and @weideng1 in #1214. Additional work will be added on as necessary to get to a working version which will support pytest as the default for running all tests. We'll make a best-efforts push to maintain compatibility with pynose but there are no guarantees on that front; in the end the only fully supported test runner will be pytest.